Skip to content

Conversation

caseyisonit
Copy link
Contributor

@caseyisonit caseyisonit commented Sep 15, 2025

Description

This PR fixes aria-label handling issues in button components and improves accessibility for pending state controllers. The changes ensure that aria-labels are properly managed and preserved across component state changes, providing better screen reader support.

Key Changes:

Button Component (@spectrum-web-components/button):

  • Fixed timing of aria-label updates to occur after slot content changes are processed, preventing race conditions
  • Added label property support for programmatic aria-label control in Storybook
  • Added comprehensive tests for aria-label behavior during content changes
  • Moved aria-label management logic to execute after slot content processing

PendingState Controller (@spectrum-web-components/reactive-controllers):

  • Improved aria-label caching logic to better handle dynamic label changes
  • Changed progress circle from aria-valuetext to aria-label for better accessibility compliance
  • Enhanced caching mechanism to preserve user-set aria-labels during pending states
  • Added more robust logic for determining when to cache aria-labels

Motivation and context

The previous implementation had timing issues, allowing aria-label updates to occur before slot content changes were fully processed, leading to inconsistent accessibility behavior. Additionally, the pending state controller was using aria-valuetext instead of aria-label for progress indicators, which is not the recommended approach for accessibility.

These changes ensure that:

  • Screen readers receive consistent and accurate aria-label information
  • User-set aria-labels are properly preserved during component state changes
  • Progress indicators follow accessibility best practices
  • Dynamic content changes don't interfere with aria-label management

Related issue(s)

  • fixes [5620]
  • resolves SWC-1039

Screenshots (if appropriate)


Author's checklist

  • I have read the CONTRIBUTING and PULL_REQUESTS documents.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices
  • I have added automated tests to cover my changes.
  • I have included a well-written changeset if my change needs to be published.
  • I have included updated documentation if my change required it.

Reviewer's checklist

  • Includes a Github Issue with appropriate flag or Jira ticket number without a link
  • Includes thoughtfully written changeset if changes suggested include patch, minor, or major features
  • Automated tests cover all use cases and follow best practices for writing
  • Validated on all supported browsers
  • All VRTs are approved before the author can update Golden Hash

Manual review test cases

  • Button aria-label timing test
  1. Go to the button Storybook stories
  2. Set a label property on a button with slot content
  3. Change the slot content dynamically
  4. Verify that the aria-label remains consistent and doesn't get overridden
  • Pending state aria-label preservation test
  1. Go to any component that uses the PendingState controller (like buttons with pending states)
  2. Set a custom aria-label on the component
  3. Trigger the pending state
  4. Verify that the custom aria-label is cached and restored when pending state ends
  • Progress circle accessibility test
  1. Go to components with pending states
  2. Use a screen reader to navigate to the progress indicator
  3. Verify that the progress circle announces properly with aria-label instead of aria-valuetext
  • Dynamic content changes test
  1. Create a button with both slot content and a label property
  2. Dynamically change the slot content multiple times
  3. Verify that the aria-label from the label property is preserved throughout

Device review

  • Did it pass in Desktop?
  • Did it pass in (emulated) Mobile?
  • Did it pass in (emulated) iPad?

Copy link

changeset-bot bot commented Sep 15, 2025

🦋 Changeset detected

Latest commit: 7edc5d3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 84 packages
Name Type
@spectrum-web-components/button Patch
@spectrum-web-components/combobox Patch
@spectrum-web-components/picker Patch
@spectrum-web-components/progress-circle Patch
@spectrum-web-components/reactive-controllers Patch
@spectrum-web-components/action-bar Patch
@spectrum-web-components/action-button Patch
@spectrum-web-components/alert-banner Patch
@spectrum-web-components/alert-dialog Patch
@spectrum-web-components/button-group Patch
@spectrum-web-components/coachmark Patch
@spectrum-web-components/dialog Patch
@spectrum-web-components/infield-button Patch
@spectrum-web-components/picker-button Patch
@spectrum-web-components/search Patch
@spectrum-web-components/tags Patch
@spectrum-web-components/toast Patch
example-project-rollup Patch
example-project-webpack Patch
@spectrum-web-components/bundle Patch
@spectrum-web-components/action-menu Patch
@spectrum-web-components/custom-vars-viewer Patch
@spectrum-web-components/story-decorator Patch
@spectrum-web-components/accordion Patch
@spectrum-web-components/action-group Patch
@spectrum-web-components/color-area Patch
@spectrum-web-components/color-slider Patch
@spectrum-web-components/color-wheel Patch
@spectrum-web-components/contextual-help Patch
@spectrum-web-components/field-label Patch
@spectrum-web-components/icon Patch
@spectrum-web-components/menu Patch
@spectrum-web-components/meter Patch
@spectrum-web-components/number-field Patch
@spectrum-web-components/overlay Patch
@spectrum-web-components/progress-bar Patch
@spectrum-web-components/radio Patch
@spectrum-web-components/sidenav Patch
@spectrum-web-components/slider Patch
@spectrum-web-components/swatch Patch
@spectrum-web-components/tabs Patch
@spectrum-web-components/tooltip Patch
@spectrum-web-components/tray Patch
@spectrum-web-components/grid Patch
@spectrum-web-components/vrt-compare Patch
documentation Patch
@spectrum-web-components/breadcrumbs Patch
@spectrum-web-components/checkbox Patch
@spectrum-web-components/icons-ui Patch
@spectrum-web-components/icons-workflow Patch
@spectrum-web-components/table Patch
@spectrum-web-components/textfield Patch
@spectrum-web-components/popover Patch
@spectrum-web-components/truncated Patch
@spectrum-web-components/top-nav Patch
@spectrum-web-components/card Patch
@spectrum-web-components/switch Patch
@spectrum-web-components/help-text Patch
@spectrum-web-components/color-field Patch
@spectrum-web-components/field-group Patch
@spectrum-web-components/eslint-plugin Patch
@spectrum-web-components/asset Patch
@spectrum-web-components/avatar Patch
@spectrum-web-components/badge Patch
@spectrum-web-components/clear-button Patch
@spectrum-web-components/close-button Patch
@spectrum-web-components/color-handle Patch
@spectrum-web-components/color-loupe Patch
@spectrum-web-components/divider Patch
@spectrum-web-components/dropzone Patch
@spectrum-web-components/icons Patch
@spectrum-web-components/iconset Patch
@spectrum-web-components/illustrated-message Patch
@spectrum-web-components/link Patch
@spectrum-web-components/modal Patch
@spectrum-web-components/split-view Patch
@spectrum-web-components/status-light Patch
@spectrum-web-components/thumbnail Patch
@spectrum-web-components/underlay Patch
@spectrum-web-components/base Patch
@spectrum-web-components/opacity-checkerboard Patch
@spectrum-web-components/shared Patch
@spectrum-web-components/styles Patch
@spectrum-web-components/theme Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Sep 15, 2025

📚 Branch Preview

🔍 Visual Regression Test Results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

Deployed to Azure Blob Storage: pr-5730

If the changes are expected, update the current_golden_images_cache hash in the circleci config to accept the new images. Instructions are included in that file.
If the changes are unexpected, you can investigate the cause of the differences and update the code accordingly.

Copy link
Contributor

github-actions bot commented Sep 15, 2025

Tachometer results

Chrome

button permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 498 kB 49.99ms - 51.12ms - faster ✔
6% - 9%
3.29ms - 5.04ms
branch 476 kB 54.06ms - 55.38ms slower ❌
6% - 10%
3.29ms - 5.04ms
-

combobox permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 782 kB 26.35ms - 27.03ms - faster ✔
2% - 5%
0.42ms - 1.40ms
branch 757 kB 27.25ms - 27.96ms slower ❌
2% - 5%
0.42ms - 1.40ms
-

light-dom-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 782 kB 307.76ms - 312.82ms - faster ✔
4% - 6%
14.49ms - 21.01ms
branch 757 kB 325.98ms - 330.10ms slower ❌
5% - 7%
14.49ms - 21.01ms
-

picker permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 596 kB 452.33ms - 458.63ms - faster ✔
15% - 17%
78.90ms - 90.91ms
branch 558 kB 535.28ms - 545.49ms slower ❌
17% - 20%
78.90ms - 90.91ms
-

progress-circle permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 427 kB 16.56ms - 17.38ms - unsure 🔍
-7% - +0%
-1.18ms - +0.01ms
branch 405 kB 17.12ms - 17.99ms unsure 🔍
-0% - +7%
-0.01ms - +1.18ms
-
Firefox

button permalink

test-basic

Version Bytes Avg Time vs remote vs branch
npm latest 498 kB 109.39ms - 113.45ms - faster ✔
6% - 11%
6.57ms - 13.35ms
branch 476 kB 118.66ms - 124.10ms slower ❌
6% - 12%
6.57ms - 13.35ms
-

combobox permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 782 kB 43.87ms - 47.01ms - unsure 🔍
-5% - +2%
-2.45ms - +1.13ms
branch 757 kB 45.24ms - 46.96ms unsure 🔍
-3% - +5%
-1.13ms - +2.45ms
-

light-dom-test permalink

Version Bytes Avg Time vs remote vs branch
npm latest 782 kB 707.62ms - 723.18ms - unsure 🔍
-2% - +1%
-13.53ms - +8.57ms
branch 757 kB 710.03ms - 725.73ms unsure 🔍
-1% - +2%
-8.57ms - +13.53ms
-

picker permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 596 kB 1000.08ms - 1008.92ms - faster ✔
5% - 8%
50.09ms - 87.71ms
branch 558 kB 1055.12ms - 1091.68ms slower ❌
5% - 9%
50.09ms - 87.71ms
-

progress-circle permalink

basic-test

Version Bytes Avg Time vs remote vs branch
npm latest 427 kB 31.99ms - 32.97ms - faster ✔
0% - 6%
0.14ms - 1.94ms
branch 405 kB 32.77ms - 34.27ms slower ❌
0% - 6%
0.14ms - 1.94ms
-

@caseyisonit caseyisonit marked this pull request as ready for review September 19, 2025 19:46
@caseyisonit caseyisonit requested a review from a team as a code owner September 19, 2025 19:46
Comment on lines 2 to 3
'@spectrum-web-components/button': patch
'@spectrum-web-components/reactive-controllers': patch
Copy link
Contributor

@Rajdeepc Rajdeepc Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Will this be read with our current changelog script? Would you prefer adding separate changeset for each package instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are other changesets with mulitple packages, i dont think it should be an issue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what Rajdeep means is, even though this works, our change log script does not pick up multiple packages (only the first one).

Copy link
Contributor

@Rajdeepc Rajdeepc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like how this has come together.
One suggestion: Would you like to add a quick screen reader test video to the PR. This would help the original reporter (and future contributors) to verify the behavior.

Copy link
Contributor

@rubencarvalho rubencarvalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! I left one blocking comment: could we confirm the double label behavior?

@coveralls
Copy link
Collaborator

coveralls commented Sep 30, 2025

Pull Request Test Coverage Report for Build 18209190251

Details

  • 96 of 96 (100.0%) changed or added relevant lines in 5 files are covered.
  • 10 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.07%) to 97.984%

Files with Coverage Reduction New Missed Lines %
packages/picker/src/Picker.ts 10 96.48%
Totals Coverage Status
Change from base Build 18198485546: 0.07%
Covered Lines: 34074
Relevant Lines: 34596

💛 - Coveralls

Copy link
Contributor

@rubencarvalho rubencarvalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 👍 I think this is the best path forward! I just left one 1 small nit comment.

@rubencarvalho
Copy link
Contributor

Copy link
Contributor

@rubencarvalho rubencarvalho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing my approval until we get clarification on the stories :)

@rubencarvalho
Copy link
Contributor

Also, unrelated to your changes, but when we have a pending picker there is no VO indication that you can't open it (but it is effectively "disabled")

Copy link
Contributor

@nikkimk nikkimk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I tested with voice over on a button with a slotted label and an aria-label only the aria-label is read. I don't think what is read should differ from what is on screen. This fixes the initial problem, but there are some a11y pitfalls with allowing this.

Can we make sure the docs recommends not using label or aria-label if they are supplying visible text via the slot?

@rubencarvalho
Copy link
Contributor

rubencarvalho commented Oct 2, 2025

When I tested with voice over on a button with a slotted label and an aria-label only the aria-label is read. I don't think what is read should differ from what is on screen. This fixes the initial problem, but there are some a11y pitfalls with allowing this.

Can we make sure the docs recommends not using label or aria-label if they are supplying visible text via the slot?

This is a good call! But as per spec, aria-label does take precedence over slotted text, so what you saw is expected... I guess we can try and rely on our docs to steer consumers away from combining a slot label with aria-label (maybe only use it for icon-only cases? or for cases where multiple buttons in a page have the same label and you need to provide SR users a bit more context to differentiate the buttons). And a dev-time warning could help too!

But I agree this is out-of-scope for this PR and better tackled on the Button refactor! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants